C#: Favor DLLs with most recent .NET Core target framework when resolving dependencies in standalone#14045
Conversation
e11d589 to
a0cf467
Compare
…ving dependencies in standalone
a0cf467 to
554a2c2
Compare
tamasvajk
left a comment
There was a problem hiding this comment.
Looks reasonable to me. I've added some questions.
|
|
||
| var name = reader.GetString(reader.GetTypeReference((TypeReferenceHandle)mHandle).Name); | ||
|
|
||
| if (name is "TargetFrameworkAttribute") |
There was a problem hiding this comment.
Is this pattern matching equivalent to name == "TargetFrameworkAttribute"?
| if ( | ||
| decoded.FixedArguments.Length > 0 && | ||
| decoded.FixedArguments[0].Value is string value && | ||
| NetCoreAppRegex().Match(value).Groups.TryGetValue("version", out var match)) |
There was a problem hiding this comment.
This NetCoreAppRegex() call is inside a foreach. I'm not sure if there's a performance hit calling this multiple times.
There was a problem hiding this comment.
I would think not, since the regex is RegexOptions.Compiled?
|
|
||
| if (name is "TargetFrameworkAttribute") | ||
| { | ||
| var decoded = attrHandle.DecodeValue(new DummyAttributeDecoder()); |
There was a problem hiding this comment.
Should this DecodeValue call be wrapped in a try-catch? Can the DummyAttributeDecoder throw NotImplementedExceptions?
There was a problem hiding this comment.
I would actually like to be made aware if any of the NotImplementedExceptions are thrown.
There was a problem hiding this comment.
Yes, it would be good to know if this exception is thrown, but it seems a bit harsh to propagate the exception.
Maybe it is worth logging it as debug or information?
michaelnebel
left a comment
There was a problem hiding this comment.
Sorry I am late to the party! Very nice @hvitved !
| using System.Reflection; | ||
| using System.Security.Cryptography; | ||
| using System.Text; | ||
| using System.Reflection.Metadata; |
There was a problem hiding this comment.
Horrible nitpick: Format Usings 😄 (I will show myself out)
|
|
||
| if (name is "TargetFrameworkAttribute") | ||
| { | ||
| var decoded = attrHandle.DecodeValue(new DummyAttributeDecoder()); |
There was a problem hiding this comment.
Yes, it would be good to know if this exception is thrown, but it seems a bit harsh to propagate the exception.
Maybe it is worth logging it as debug or information?
Nuget packages often contain multiple DLLs that target different .NET frameworks. With this PR, the standalone extractor will always choose the DLL that targets the most recent .NET Core version.